Skip to content

London |26-ITP-Sep| Maharit Belai| Sprint 2 | Implement and rewrite tests - #1716

Open
MaharitB wants to merge 50 commits into
CodeYourFuture:mainfrom
MaharitB:implement-and-rewrite-tests
Open

MaharitB wants to merge 50 commits into
CodeYourFuture:mainfrom
MaharitB:implement-and-rewrite-tests

Conversation

@MaharitB

@MaharitB MaharitB commented Oct 15, 2026 •

Copy link
Copy Markdown

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Task code

CYF-1059

Changelist

I have implemented and wrote tests.

Questions

…the input can be posetive before they are compared
@github-actions github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 15, 2026
@MaharitB MaharitB added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 15, 2026
@hey-hammad hey-hammad added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 20, 2026

@hey-hammad hey-hammad left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good effort overall, a few issues requires attention, regarding code quality and test cases. Thanks


export function isProperFraction(numerator, denominator) {
// TODO: Implement this function
if (Math.abs(numerator) < Math.abs(denominator) && numerator !== 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If 0,5 is passed to the fn, what is will be the output of the function? is it the correct output?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also please add the above input as one of the test cases.

@@ -24,5 +24,52 @@
// execute the code to ensure all tests pass.

export function getCardValue(card) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall code is good and works fine, however few things can be improved to make it better.

  • do we need to use let for defining the suit and cardInput? do we expect the value of these variables to be changed later? If not is there another way of defining the variables that's better suited for this scenario?

"K",
];

if (suit == "♠" || suit == "♥" || suit == "♣" || suit == "♦") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you already have all the validSuit defined in an array, would it make more sense to check the if the suit already exist in the array rather than checking individually?

return 10;
}
}
if (card == "invalid") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it better to perform all the validation before or after any calculations? which is better?

// TODO: Write tests in Jest syntax to cover all possible outcomes.

// Case 1: Ace (A)
test(`Should return 11 when given an ace card`, () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've added a ton of validation for checking if the input is correct, wouldn't it make more sense to test it to confirm if its working fine?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think about malformed inputs etc.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the constructive feedback . I really appreciate it.
I included tests that prove if the validation works.It is at the bottom the of test file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaharitB couple of test including malformed inputs like having empty spaces at the start or end of string will make the test suits more robust. Thanks

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification . i have made the changes based on your advice.

@hey-hammad hey-hammad added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 20, 2026
@MaharitB MaharitB added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 21, 2026

@hey-hammad hey-hammad left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good work on fixing the previous issues, we're almost there, couple of test cases needs attention.

test(`should return true when (denominator < numerator)`, () => {
expect(isProperFraction(1, -2)).toEqual(true);
});
test(`should return false when(numerator = 0)`, () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please review this test case again

"Expected clear error"
);
});
test("throws invalid input when given null as input", () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is null and empty space the same in javascript?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hey-hammad No,they are not the same. empty space is considered a character and null is nothing.
I will have a look on the code. Thank you for raising it.

@hey-hammad hey-hammad added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 23, 2026
@MaharitB MaharitB added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants